Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove caseSensitivityKey invalidation #20965

Merged
merged 13 commits into from
Jul 14, 2023
Merged

Remove caseSensitivityKey invalidation #20965

merged 13 commits into from
Jul 14, 2023

Conversation

akshya96
Copy link
Contributor

@akshya96 akshya96 commented Jun 2, 2023

Jira issue: https://hashicorp.atlassian.net/browse/VAULT-16845
Ent PR: https://github.com/hashicorp/vault-enterprise/pull/4233

A problem occurs when the caseSensitivityKey is invalidated due to a change in its value from True to False in storage, and duplicate groups are discovered. As a result, the loadGroups function returns an error, preventing the loading of groups and potentially causing missing groups in the memDB.

To resolve this issue, the proposed solution is to eliminate the invalidation case for the caseSensitivityKey. Since the value in storage can only be changed on the primary active node during initialization, there is no need for further invalidation. A warning is anyways being logged in loadIdentityStoreArtifacts function during the unseal process before initialization, if duplicate groups are encountered.

@davidadeleon
Copy link
Contributor

If we are removing the invalidation for this key in storage, should we also look to remove the key from storage in general moving forward? The only place we update this value in storage is here:

entry, err := logical.StorageEntryJSON(caseSensitivityKey, &casesensitivity{
DisableLowerCasedNames: i.disableLowerCasedNames,
})
if err != nil {
return err
}
return i.view.Put(ctx, entry)

Which looks to also be (after this change) the last reference for this struct:
type casesensitivity struct {
DisableLowerCasedNames bool `json:"disable_lower_cased_names"`
}

And this variable:
caseSensitivityKey = "casesensitivity"

@maxb
Copy link
Contributor

maxb commented Jun 7, 2023

Indeed, isn't acting as an invalidation signal the whole reason for this key's existence?

Are you willing/able to share more reason about this change publicly? Once upon a time (years ago), with a previous employer, I witnessed a spontaneous change in case sensitivity from False to True in a production cluster, and we never did figure out how that was even possible. I'm kind of curious whether more information has come to light! :-)

@akshya96
Copy link
Contributor Author

akshya96 commented Jun 7, 2023

If we are removing the invalidation for this key in storage, should we also look to remove the key from storage in general moving forward? The only place we update this value in storage is here:

entry, err := logical.StorageEntryJSON(caseSensitivityKey, &casesensitivity{
DisableLowerCasedNames: i.disableLowerCasedNames,
})
if err != nil {
return err
}
return i.view.Put(ctx, entry)

Which looks to also be (after this change) the last reference for this struct:

type casesensitivity struct {
DisableLowerCasedNames bool `json:"disable_lower_cased_names"`
}

And this variable:

caseSensitivityKey = "casesensitivity"

I agree that since the invalidation of the key is removed, we can remove the key itself but I think we will still need this field in the identitystore because it uses it to create the schema. I updated the PR with the changes.

@ncabatoff
Copy link
Collaborator

Are you willing/able to share more reason about this change publicly? Once upon a time (years ago), with a previous employer, I witnessed a spontaneous change in case sensitivity from False to True in a production cluster, and we never did figure out how that was even possible. I'm kind of curious whether more information has come to light! :-)

Until #20964 we allowed for duplicate groups to be created due to a bug. Once duplicate groups exist, on load we modify the in-memory setting, then during the initialize call of the identitystore we'll modify the stored setting.

@raskchanky
Copy link
Contributor

I read the Slack discussion and the Jira and removing invalidation makes sense given the key isn't supposed to change, but I'm not following the part about removing the bits from initialize() that write the key to storage. If it's not in storage, where does the default value come from?

@davidadeleon
Copy link
Contributor

davidadeleon commented Jun 9, 2023

I read the Slack discussion and the Jira and removing invalidation makes sense given the key isn't supposed to change, but I'm not following the part about removing the bits from initialize() that write the key to storage. If it's not in storage, where does the default value come from?

The IdentityStore struct has its own variable for case sensitivity which defaults to false. When we initialize a new Identity store it will be false, and only internally set to true if we find any duplicate groups when we load them on initialization.

disableLowerCasedNames bool

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the background info I read, I think this is ok?

DisableLowerCasedNames: i.disableLowerCasedNames,
})
// if the storage entry for caseSensitivityKey exists, remove it
storageEntry, err := i.view.Get(ctx, caseSensitivityKey)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a Get, you can just Delete - it won't return an error if the thing is already missing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although now that I think about it, let's keep this, and add a log line where we record the value it had at time of deletion. Could be useful in debugging at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added debug line to get caseSensitivityKey value at the time of deletion

@akshya96 akshya96 requested a review from ncabatoff June 21, 2023 17:30
vault/identity_store.go Outdated Show resolved Hide resolved
@VioletHynes VioletHynes added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants